-
Notifications
You must be signed in to change notification settings - Fork 502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: eager loading of wasm in jest environment #1246
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Can you add a simple test with Jest? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See suggestions. Needs to work on node 12.
4d77a03
to
845c757
Compare
package.json
Outdated
@@ -48,7 +48,7 @@ | |||
"test": "npm run test:tap && npm run test:node-fetch && npm run test:fetch && npm run test:jest && tsd", | |||
"test:node-fetch": "node scripts/verifyVersion.js 16 || mocha test/node-fetch", | |||
"test:fetch": "node scripts/verifyVersion.js 16 || tap test/fetch/*.js", | |||
"test:jest": "jest test/jest/test", | |||
"test:jest": "jest test/jest/*.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcollina found there was a test file, mock-agent.test.js, for jest, that had been disabled, and by changing this and it running, we actually see this error, and with this fix its resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -408,6 +408,8 @@ const constants = require('./llhttp/constants') | |||
const EMPTY_BUF = Buffer.alloc(0) | |||
|
|||
async function lazyllhttp () { | |||
const llhttpWasmData = process.env.JEST_WORKER_ID ? require('./llhttp/llhttp.wasm.js') : undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I don't understand? What's the purpose? This isn't very eager.
@ronag I agree, its not very clear to be exactly whats causing this, but after the first version, i thought, lets just try this, and works fine in the projects that failed. I guess its related to doing |
I guess im done changing it, the last change got windows to find the tests, looks like wont run on macos because of my access level, but i've run it on macos myself |
fixes #1235
Its pretty horrible, and but really need a solution to this problem.